Conversation
leoguignard
left a comment
leoguignard
left a comment
There was a problem hiding this comment.
Different proposition on how to solve the bug, and some small suggestions on how to improve the code
| t = lT.t_e | ||
| to_do = list(r) | ||
| to_do = [] | ||
| final_nodes = [] | ||
| for root in r: | ||
| if lT.time[root] == t: | ||
| final_nodes.append(root) | ||
| else: | ||
| to_do.append(root) | ||
| while len(to_do) > 0: | ||
| curr = to_do.pop() | ||
| for _next in lT._successor[curr]: |
There was a problem hiding this comment.
My suggestion, keeping as is, only changing the while:
while 0 < len(to_do):
curr = to_do.pop()
if lT._time[curr] == t:
final_nodes.append(curr)
elif lT._time[curr] < t:
to_do.extend(lT.successor[curr])I think it also allows the removal of
if not final_nodes:
return list(r)| @@ -366,8 +366,13 @@ def nodes_at_t( | |||
| r = [r] | |||
There was a problem hiding this comment.
Also I guess one could do it that way at the beginning:
if r is None:
...Instead of
if not r and r != 0:
...There was a problem hiding this comment.
And yes, it would change the behavior since it would make the function return nothing in case the user gave [] for roots instead of returning the output of lT.roots which is I think a plus.
If the user specifically gave the empty list of nodes, it should not get any node in the return value.
There was a problem hiding this comment.
Since we have time_nodes again, I don't care about the behaviour, so I will do that.
There was a problem hiding this comment.
Actually since we have the time_nodes the function should change entirely.
There was a problem hiding this comment.
Also, I think we might want to return a set and not a list. There is no reason to index the result, and I like sets more. The only reason to use a list is that the rest of the LineageTree consists mostly of lists, so we can do so for the sake of consistency.
There was a problem hiding this comment.
Also I guess one could do it that way at the beginning:
if r is None:
...Instead of
if not r and r != 0:
...
Dont you want to do that?
There was a problem hiding this comment.
Why should the function change? and what should it be instead?
About the list vs set, the list can have an advantadge since it is explicitly ordered and that currently the nodes are returned in a specific order (preorder)
There was a problem hiding this comment.
If we dont provide roots it should return lT.time_nodes[t]
| @@ -366,8 +366,13 @@ def nodes_at_t( | |||
| r = [r] | |||
There was a problem hiding this comment.
Also I guess one could do it that way at the beginning:
if r is None:
...Instead of
if not r and r != 0:
...
Dont you want to do that?
| @@ -366,8 +366,13 @@ def nodes_at_t( | |||
| r = [r] | |||
There was a problem hiding this comment.
Why should the function change? and what should it be instead?
About the list vs set, the list can have an advantadge since it is explicitly ordered and that currently the nodes are returned in a specific order (preorder)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## code-split-3 #85 +/- ##
=============================================
Coverage 96.91% 96.91%
=============================================
Files 2 2
Lines 356 356
Branches 17 17
=============================================
Hits 345 345
Misses 6 6
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
added to Closing for that reason |
3 participants
If some of the roots existed in t while others started later, the ones that existed would not be added to the result.